Sign update #5797

TheLimeGlass · 2023-07-07T18:07:37Z

Description

Updated ExprSignText to modern standards.
Added Skript's ChatFormat support with Adventure API.
Added support for multiple signs.
Did all the versioning for 1.20 where there can now be two sides to a sign.
Added changers as Njol's comment from 9 years stated to do.

Notes:

This class is a large review due to versioning, adventure api AND 1.20 changes. o7
BlockState#update(boolean, boolean) is 1.13+ so no need for try/catch anymore, also made the force boolean true because the user is modifying the value.
I have an aliases update for 1.20 covering many sign features Aliases for 1.20 skript-aliases#88

Target Minecraft Versions: any
Requirements: Paper is optional
Related Issues: #4576 #5778

src/main/java/ch/njol/skript/classes/data/BukkitClasses.java

src/main/java/ch/njol/skript/expressions/ExprSignText.java

Co-authored-by: Ayham Al Ali <[email protected]>

TheLimeGlass · 2024-02-14T20:21:16Z

Changed to a functional interface to support:

Sign::getLines
Sign::getLine
Sign::setLine
Sign::line
SignChangeEvent::getLines
SignChangeEvent::getLine
SignChangeEvent::setLine
SignChangeEvent::line
SignSide::getLines
SignSide::getLine
SignSide::setLine
SignSide::line

Tests failing is not related to this pull request. Fix at #6433

…ript into feature/line-signs

src/main/java/ch/njol/skript/classes/Changer.java

src/main/java/ch/njol/skript/classes/data/BukkitClasses.java

src/main/java/ch/njol/skript/classes/data/BukkitEventValues.java

APickledWalrus · 2024-08-16T22:27:36Z

src/main/java/ch/njol/skript/expressions/ExprSignText.java

+
+		void setLine(int line, @NotNull String value);
+
+		static void setLine(SetSignLine<Integer, String> setLineMethod, int line, String value) {


What is the point of these static methods? Why not just call setLineMethod.setLine(line, value) directly? Rather than all of these interfaces, I think it would be better just to have a Spigot and Adventure implementations of an interface with set/get methods. The Adventure implementation can handle the string->component conversions.

It's required to be static, because that's how FunctionalInterface works. One method must be static.

Adventure doesn't exist on Spigot, so you can't rely on Adventure to handle the string to component conversions as the method doesn't exist.

It's a functional interface because AdventureSetSignLine has to be a separate class due to the classes not existing and needing to be statically initialized.

There should be no problem to call the interface methods directly? There's no requirement for there be a static method.

If the Adventure implementation is used only on Paper, I don't see why it would be an issue to handle the component parsing in the interface implantation rather than the syntax's change method. I think the implementation right now is somewhat over complicated for what we need

I don't agree with it being more complex. It's making it simpler to read. You have to deal with all the provided methods I posted #5797 (comment)

I think it's unnecessary for the Adventure interface to be something we're specifically checking for in change. There should just be an implementation for Adventure of SetSignLine that handles the component conversion (that way adventure doesn't need passed/handled as it's own parameter). Additionally, I'm still not seeing why these static methods are necessary. If you have the interface object, why can't you just call the method it has directly?

Getting back to this, I understand the different interfaces, and I see their usefulness, but I don't understand why we can't just use the interface directly. That is, instead of:

SetSignLine.setLine(SET_LINE, i, value)

just call

SET_LINE.setLine(i, value)

Is there a reason this wouldn't work? If there is some sort of error, can you reply with it?

src/main/java/ch/njol/skript/expressions/ExprSignText.java

…ript into feature/line-signs

APickledWalrus · 2024-12-17T17:14:30Z

src/main/java/ch/njol/skript/expressions/ExprSignText.java

+
+		void setLine(int line, @NotNull String value);
+
+		static void setLine(SetSignLine<Integer, String> setLineMethod, int line, String value) {


Getting back to this, I understand the different interfaces, and I see their usefulness, but I don't understand why we can't just use the interface directly. That is, instead of:

SetSignLine.setLine(SET_LINE, i, value)

just call

SET_LINE.setLine(i, value)

Is there a reason this wouldn't work? If there is some sort of error, can you reply with it?

APickledWalrus · 2024-12-17T17:16:01Z

src/main/java/ch/njol/skript/expressions/ExprSignText.java

 	static {
+		if (Skript.isRunningMinecraft(1, 19, 4))
+			serializer = BungeeComponentSerializer.get();
+		String addition = RUNNING_1_20 ? "[[on [the] (front|:back) side] of [sign[s]] %blocks%]" : "[of [sign[s]] %blocks%]";


Could we just use the SignSide type instead?

APickledWalrus · 2024-12-17T17:17:50Z

src/main/java/ch/njol/skript/expressions/ExprSignText.java

-		if (line < 0 || line > 3)
+	protected String[] get(Event event) {
+		int line = 0;
+		if (this.line == null && !multipleLines) {


Based on init, this will never be true?

APickledWalrus · 2024-12-17T17:18:41Z

src/main/java/ch/njol/skript/expressions/ExprSignText.java

-
-	@SuppressWarnings({"unchecked", "null"})
+	private Expression<Block> blocks;
+	private boolean multipleLines;


Is this not implied by line != null?

APickledWalrus · 2024-12-17T17:20:28Z

src/main/java/ch/njol/skript/expressions/ExprSignText.java

-		return null;
+	public Class<?>[] acceptChange(ChangeMode mode) {
+		boolean acceptsMany = line == null;
+		switch (mode) {


Can now return a switch expression

APickledWalrus · 2024-12-17T17:21:25Z

src/main/java/ch/njol/skript/expressions/ExprSignText.java

+	}
+
+	@Override
+	public String toString(@Nullable Event event, boolean debug) {


Should support the side field

Add side, multiple lines, chat formats and more to signs

2f87ec8

TheLimeGlass added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Jul 7, 2023

TheLimeGlass added 2 commits July 7, 2023 12:18

Add sign side classinfo

22e30f8

Attempt to fix error

9e22b00

TheLimeGlass marked this pull request as draft July 7, 2023 19:23

TheLimeGlass added 2 commits July 7, 2023 13:26

Remove debug message

6301ef2

Attempt to fix error

0345a3f

TheLimeGlass marked this pull request as ready for review July 7, 2023 19:35

Fix error

60d4fd6

NotSoDelayed mentioned this pull request Jul 14, 2023

Enhance & Fix Sign Text Expression #5465

Closed

TheLimeGlass requested review from AyhamAl-Ali and APickledWalrus August 30, 2023 11:48

TheLimeGlass and others added 2 commits August 30, 2023 05:50

Merge branch 'master' into feature/line-signs

6ca69d7

Merge branch 'master' into feature/line-signs

7cd5193

Moderocky requested changes Sep 8, 2023

View reviewed changes

src/main/java/ch/njol/skript/classes/data/BukkitClasses.java Outdated Show resolved Hide resolved

Moderocky force-pushed the master branch 3 times, most recently from bd134d0 to 3f08853 Compare September 16, 2023 16:59

TheLimeGlass changed the base branch from master to dev/feature October 14, 2023 11:16

Update BukkitClasses.java

2252c7b

TheLimeGlass requested a review from Moderocky February 14, 2024 04:52

Merge branch 'dev/feature' into feature/line-signs

67dea3b

AyhamAl-Ali requested changes Feb 14, 2024

View reviewed changes

sovdeeth requested changes Feb 14, 2024

View reviewed changes

TheLimeGlass and others added 4 commits February 14, 2024 10:07

Update src/main/java/ch/njol/skript/expressions/ExprSignText.java

8385d03

Co-authored-by: Ayham Al Ali <[email protected]>

Make a functional interface

9141b49

Re-add broken compile

2776bd1

Remove debug

922fb2a

TheLimeGlass requested review from AyhamAl-Ali and sovdeeth February 14, 2024 20:19

sovdeeth and others added 5 commits February 14, 2024 21:47

Merge branch 'dev/feature' into feature/line-signs

3098055

Remove deprecated method

da44ef6

Merge branch 'feature/line-signs' of https://github.com/SkriptLang/Sk…

19e570c

…ript into feature/line-signs

Merge branch 'dev/feature' into feature/line-signs

ead4234

Merge branch 'dev/feature' into feature/line-signs

e27a78f

Moderocky approved these changes May 3, 2024

View reviewed changes

TheLimeGlass and others added 2 commits May 10, 2024 13:09

Merge branch 'dev/feature' into feature/line-signs

fccdbec

Merge branch 'dev/feature' into feature/line-signs

3c1a20c

APickledWalrus requested changes Aug 16, 2024

View reviewed changes

TheLimeGlass added 2 commits August 31, 2024 08:58

Apply changes

69c9e6e

Merge branch 'feature/line-signs' of https://github.com/SkriptLang/Sk…

62fbeb7

…ript into feature/line-signs

TheLimeGlass requested a review from APickledWalrus August 31, 2024 15:00

Resolve conflicts

c54ff14

APickledWalrus requested changes Dec 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sign update #5797

Sign update #5797

TheLimeGlass commented Jul 7, 2023 •

edited

Loading

TheLimeGlass commented Feb 14, 2024 •

edited

Loading

APickledWalrus Aug 16, 2024

TheLimeGlass Aug 31, 2024 •

edited

Loading

APickledWalrus Aug 31, 2024

TheLimeGlass Aug 31, 2024

APickledWalrus Sep 1, 2024 •

edited

Loading

APickledWalrus Dec 17, 2024

APickledWalrus Dec 17, 2024

APickledWalrus Dec 17, 2024

APickledWalrus Dec 17, 2024

APickledWalrus Dec 17, 2024

APickledWalrus Dec 17, 2024

APickledWalrus Dec 17, 2024


		void setLine(int line, @NotNull String value);

		static void setLine(SetSignLine<Integer, String> setLineMethod, int line, String value) {

Sign update #5797

Are you sure you want to change the base?

Sign update #5797

Conversation

TheLimeGlass commented Jul 7, 2023 • edited Loading

Description

TheLimeGlass commented Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

TheLimeGlass Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

APickledWalrus Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheLimeGlass commented Jul 7, 2023 •

edited

Loading

TheLimeGlass commented Feb 14, 2024 •

edited

Loading

TheLimeGlass Aug 31, 2024 •

edited

Loading

APickledWalrus Sep 1, 2024 •

edited

Loading